Skip to content

fix: resolve stream ID from event store metadata in InMemoryEventStore - #3149

Open
zhouyunding wants to merge 1 commit into
modelcontextprotocol:mainfrom
zhouyunding:fix/2560-eventstore-streamid-resume
Open

zhouyunding wants to merge 1 commit into
modelcontextprotocol:mainfrom
zhouyunding:fix/2560-eventstore-streamid-resume

Conversation

@zhouyunding

Copy link
Copy Markdown

Fixes #2560

Root cause

InMemoryEventStore.getStreamIdFromEventId derived the stream ID by parsing the
event ID (eventId.split('_')[0]). Event IDs have the form
${streamId}_${timestamp}_${rand}, which is not a reversible encoding when the
stream ID itself contains underscores:

  • _GET_stream (the standalone GET stream) → parsed as ''
  • stream_1 → truncated to stream

Impact (measured)

With a real WebStandardStreamableHTTPServerTransport + this store, resuming a
standalone GET stream via Last-Event-ID returns 200 text/event-stream but the
stream is closed immediately (0 ms) with zero frames, and the successor stream is
never registered under _GET_stream — every later server-initiated notification
on that connection is silently dropped. The default client retries
(maxRetries: 2) and then gives up. Replaying already-stored events is also
empty, because replay filters under the wrong stream ID.

Scope note: _GET_stream is the only underscore-leading stream ID used by the
shipped transport (request streams use crypto.randomUUID()); the truncation
path only affects direct users of this exported example store whose stream IDs
contain underscores.

Fix

Read the stream ID from the store's own metadata instead of parsing the event ID:

return this.events.get(eventId)?.streamId ?? '';

The store already persists { streamId, message }, so this is the single
unambiguous source of truth (the "explicit eventId→streamId mapping" option from
the issue). String parsing — including lastIndexOf — cannot work in general,
because the random suffix may itself contain underscores.

Behavior change

  • replayEventsAfter now returns the correct stream ID and replays subsequent
    events for _GET_stream and any stream ID containing underscores.
  • Unknown lastEventId / empty store still return '' (unchanged).
  • No interface or signature changes; the EventStore contract is untouched.

Testing (both sides)

  • RED (base implementation + the new tests verbatim): 2 failed / 2 passed —
    expected '' to be '_GET_stream', expected 'stream' to be 'stream_1'.
  • GREEN (with fix): 4/4 new tests pass; package suite 2 files / 6 tests pass;
    build:all, lint:all, pnpm -r typecheck, transportResumability (3/3),
    hosting-resume e2e (14/14) all pass.
  • Transport-level probe (real transport + real store): base → resume closed at
    0 ms, no frames, mappingKeys=[]; fixed → replays the subsequent event,
    mapping key _GET_stream, stream stays open >500 ms.

Regression protection is unit-level (examples/shared); the repo's
integration/e2e doubles use the same split('_')[0] pattern and never exercise
underscore stream IDs — intentionally left out of scope.

Known limitations (pre-existing, not introduced by this PR)

  • Same-millisecond events can replay out of order or be dropped (eventId string
    sort includes the random suffix); measured locally, deserves its own issue.
  • No changeset: @mcp-examples/shared is private: true and listed in
    .changeset/config.json ignore.

AI disclosure

This PR was developed with AI assistance (implementation, tests, and independent
reviews by separate AI reviewers under human supervision). All changes were
verified locally before submission (RED→GREEN tests, lint, typecheck, e2e).

`getStreamIdFromEventId()` recovered the stream ID with
`eventId.split('_')[0]`, which returns an empty string for stream IDs
that start with an underscore (the standalone GET stream `_GET_stream`)
and truncates IDs containing an underscore (`stream_1` becomes `stream`).
As a result `replayEventsAfter()` returned an empty stream ID and replayed
events were silently dropped after a resume.

Read the stream ID from the event's stored metadata instead of parsing it
out of the event ID, whose format is an internal implementation detail.
@zhouyunding
zhouyunding requested a review from a team as a code owner November 4, 2026 16:15
@changeset-bot

changeset-bot Bot commented Nov 4, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: 382f7d0

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@pkg-pr-new

pkg-pr-new Bot commented Nov 4, 2026

Copy link
Copy Markdown

Open in StackBlitz

@modelcontextprotocol/client

npm i https://pkg.pr.new/@modelcontextprotocol/client@3149

@modelcontextprotocol/codemod

npm i https://pkg.pr.new/@modelcontextprotocol/codemod@3149

@modelcontextprotocol/core

npm i https://pkg.pr.new/@modelcontextprotocol/core@3149

@modelcontextprotocol/server

npm i https://pkg.pr.new/@modelcontextprotocol/server@3149

@modelcontextprotocol/server-legacy

npm i https://pkg.pr.new/@modelcontextprotocol/server-legacy@3149

@modelcontextprotocol/express

npm i https://pkg.pr.new/@modelcontextprotocol/express@3149

@modelcontextprotocol/fastify

npm i https://pkg.pr.new/@modelcontextprotocol/fastify@3149

@modelcontextprotocol/hono

npm i https://pkg.pr.new/@modelcontextprotocol/hono@3149

@modelcontextprotocol/node

npm i https://pkg.pr.new/@modelcontextprotocol/node@3149

commit: 382f7d0

@claude claude Bot added the v2 Ideas, requests and plans for v2 of the SDK which will incorporate major changes and fixes label Nov 5, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v2 Ideas, requests and plans for v2 of the SDK which will incorporate major changes and fixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

InMemoryEventStore: standalone-stream resume broken by getStreamIdFromEventId split

1 participant